Skip to content

Conversation

assemblaj
Copy link
Contributor

@assemblaj assemblaj commented Oct 7, 2019

let result_greater_vals = vec![1., 2., 4.].into_iter().collect::<VecDeque<f64>>()
.partial_cmp(vec![1., 2., 3.].into_iter().collect::<VecDeque<f64>>()).await;
let result_none = vec![std::f64::NAN].into_iter().collect::<VecDeque<f64>>()
.partial_cmp(vec![1.].into_iter().collect::<VecDeque<f64>>()).await;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is quite the comparison haha. I'm wondering if we could perhaps move most of this to a dedicated test under src/stream/stream/partial_cmp.rs, and then add a more reduced example as part of the doc comment. That way we keep the thoroughness of the tests to catch regressions while gaining legibility.

I'm afraid that in its current form folks might be somewhat overwhelmed by the documentation, which is something we generally want to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR was branched from assemblaj-partial_cmp, that's the example code for partial_cmp. I can go back and update the partial_cmp example it to be cleaner and then update this branch.

The issue I was having is that the std::iter example code for partial_cmp just creates and compares the iterators inline; doing this with VecDeque's doesn't look as clean but I'm not sure about making a few at the top of the function and copying them everywhere either (this is what I did for the Stream::ge examples, see below). How do you think I should approach this?

let single:     VecDeque<isize> = vec![1].into_iter().collect();
let single_gt:  VecDeque<isize> = vec![10].into_iter().collect();
let multi:      VecDeque<isize> = vec![1,2].into_iter().collect();
let multi_gt:   VecDeque<isize> = vec![1,5].into_iter().collect();

assert_eq!(single.clone().ge(single.clone()).await, true);
assert_eq!(single_gt.clone().ge(single.clone()).await, true);

assert_eq!(multi.clone().ge(single_gt.clone()).await, false);
assert_eq!(multi_gt.clone().ge(multi.clone()).await, true);

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this patch looks really good! -- left a small nit, but I think we should be good to merge once that's addressed!

edit: this PR also needs to be rebased

@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Oct 8, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ghost ghost merged commit a7041be into async-rs:master Oct 15, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants